Skip to content

Conversation

@jtomaszewski
Copy link
Contributor

Summary

  • Add skipConnectSpans configuration option to skip creating pg.connect and pg-pool.connect spans
  • Query spans and pool metrics are still recorded when this option is enabled

Background

The pg.connect and pg-pool.connect spans measure connection time but provide limited actionable insight in many scenarios:

  • They don't correlate to specific queries
  • Query spans already include total time (including connection wait)
  • High-traffic scenarios create many connect spans per request

Changes

  • Add skipConnectSpans?: boolean to PgInstrumentationConfig (default false)
  • Skip span creation in both _getClientConnectPatch() and _getPoolConnectPatch() when enabled
  • Still call _setPoolConnectEventListeners() to preserve pool metrics
  • Add tests verifying both span types are skipped and metrics are preserved
  • Update README with new option documentation

Test plan

  • Added unit tests for skipConnectSpans=true verifying no connect spans are created
  • Added test verifying pool metrics are still recorded when skipping spans
  • Verified existing tests still pass

🤖 Generated with Claude Code

Add a configuration option to skip creating `pg.connect` and `pg-pool.connect`
spans while preserving query spans and pool metrics. This helps reduce trace
noise in high-throughput scenarios where connection time is not of interest.

- Add `skipConnectSpans?: boolean` to PgInstrumentationConfig
- Skip span creation for both client and pool connect operations
- Still set up event listeners for pool metrics when skipping spans
- Add tests for new option
- Update README with new option documentation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@maryliag
Copy link
Contributor

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a name suggestion, following the name convention we use in other instrumentations: skip for internal functions, and ignore on user facing configuration (e.g. ignoreResolveSpans)

| `responseHook` | `PgInstrumentationExecutionResponseHook` (function) | Function for adding custom span attributes from db response |
| `requireParentSpan` | `boolean` | If true, requires a parent span to create new spans (default false) |
| `addSqlCommenterCommentToQueries` | `boolean` | If true, adds [sqlcommenter](https://github.com/open-telemetry/opentelemetry-sqlcommenter) specification compliant comment to queries with tracing context (default false). _NOTE: A comment will not be added to queries that already contain `--` or `/* ... */` in them, even if these are not actually part of comments_ |
| `skipConnectSpans` | `boolean` | If true, `pg.connect` and `pg-pool.connect` spans will not be created. Query spans and pool metrics are still recorded (default false) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `skipConnectSpans` | `boolean` | If true, `pg.connect` and `pg-pool.connect` spans will not be created. Query spans and pool metrics are still recorded (default false) |
| `ignoreConnectSpans` | `boolean` | If true, `pg.connect` and `pg-pool.connect` spans will not be created. Query spans and pool metrics are still recorded (default false) |

if (utils.shouldSkipInstrumentation(plugin.getConfig())) {
const config = plugin.getConfig();

if (utils.shouldSkipInstrumentation(config) || config.skipConnectSpans) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (utils.shouldSkipInstrumentation(config) || config.skipConnectSpans) {
if (utils.shouldSkipInstrumentation(config) || config.ignoreConnectSpans) {

// Still set up event listeners for metrics even when skipping spans
plugin._setPoolConnectEventListeners(this);

if (config.skipConnectSpans) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (config.skipConnectSpans) {
if (config.ignoreConnectSpans) {

*
* @default false
*/
skipConnectSpans?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
skipConnectSpans?: boolean;
ignoreConnectSpans?: boolean;

assert.strictEqual(spans.length, 0);
});

it('should not create connect spans when skipConnectSpans=true', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should not create connect spans when skipConnectSpans=true', async () => {
it('should not create connect spans when ignoreConnectSpans=true', async () => {

it('should not create connect spans when skipConnectSpans=true', async () => {
const newPool = new pgPool(CONFIG);
create({
skipConnectSpans: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
skipConnectSpans: true,
ignoreConnectSpans: true,

assert.ok(querySpans.length > 0, 'Expected query spans to be created');
});

it('should still record pool metrics when skipConnectSpans=true', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should still record pool metrics when skipConnectSpans=true', async () => {
it('should still record pool metrics when ignoreConnectSpans=true', async () => {

const metricReader = testUtils.initMeterProvider(instrumentation);
const newPool = new pgPool(CONFIG);
create({
skipConnectSpans: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
skipConnectSpans: true,
ignoreConnectSpans: true,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants